Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli) - Runtime binary for Deno #8944

Closed
wants to merge 23 commits into from
Closed

Conversation

yos1p
Copy link
Contributor

@yos1p yos1p commented Dec 31, 2020

deno_runtime crate will produce lite binary: deno-rt in target folder (#8757)

CC @bartlomieju @lucacasonato

@lucacasonato
Copy link
Member

This looks good, but could you split the PR so this PR only adds the deno-rt binary? Then we can change how deno compile works in a follow up PR.

@yos1p yos1p marked this pull request as ready for review December 31, 2020 13:08
@yos1p
Copy link
Contributor Author

yos1p commented Dec 31, 2020

Okay. I reverted some of the changes in Deno compile. I also move some of the code from cli/standalone.rs to runtime/standalone.rs. Running from CLI should will call run function from runtime crate now.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yos1p looks good, nice work!

That said I'm not sure if we should add a binary target to deno_runtime, maybe instead we should add a second binary target to cli? (I know I suggested to put it in the runtime dir in the original issue, but I'm having doubts now). With this change concepts of standalone binaries are split into cli and runtime and I believe it's preferable to have all of that code in cli.

@ry WDYT?

@bartlomieju
Copy link
Member

Also we need to track the size of the "lite" binary in the benchmarks: https://deno.land/benchmarks#executable-size I'll be fine to add that in a separate PR though.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - but can we bike shed on the name of executable a bit?

Generally we avoid dashes in filenames. "rt" is not very intelligible.

What about denolite ?

@bartlomieju
Copy link
Member

Looks good - but can we bike shed on the name of executable a bit?

Generally we avoid dashes in filenames. "rt" is not very intelligible.

What about denolite ?

Good point, +1 for denolite

@lucacasonato
Copy link
Member

denolite sounds like a lite version of Deno for general use. But it is not. It is purely the runtime for standalone executable. I would prefer denort or denoruntime.

@yos1p
Copy link
Contributor Author

yos1p commented Dec 31, 2020

Thank you for review! Okay. I'll try to move the code and binary to be generated by cli crate.

Also we need to track the size of the "lite" binary in the benchmarks: https://deno.land/benchmarks#executable-size I'll be fine to add that in a separate PR though.

I can do it in this PR.

For the naming, I kind of prefer denort instead of denolite, but name can be easily changed later. I'll just follow what you guys decide.

@yos1p yos1p marked this pull request as draft December 31, 2020 17:52
@yos1p yos1p changed the title Lite binary for Deno feat(cli) - Runtime binary for Deno Jan 1, 2021
cli/standalone.rs Outdated Show resolved Hide resolved
@yos1p yos1p marked this pull request as ready for review January 1, 2021 03:59
@yos1p yos1p requested review from ry and bartlomieju January 5, 2021 08:29
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good @yos1p.

When I run denort in without an arguments I get a zero exit code (success) and no output. I think this is going to be confusing to people who don't know about this. It would be better to print an error message and have a non-zero exit code.

cli/denort.rs Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to have a test of some sort... How will we know if this thing breaks?

@yos1p
Copy link
Contributor Author

yos1p commented Jan 6, 2021

I think we need to have a test of some sort... How will we know if this thing breaks?

I think we can use similar tests like standalone, but this can only be applied when we already implement compile with denort instead of deno.

cli/denort.rs Outdated

// TODO (yos1p) Specify better error message
eprintln!(
"{}: Runtime Error.",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry Any suggestions for the error message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yos1p this is fine for now, I will review this PR in more detail today/tomorrow and merge it. @lucacasonato and I will look into using this new binary target for standalone executables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This executable is used internally by 'deno compile', it is not meant to be invoked directly."

@bartlomieju
Copy link
Member

@yos1p thank you for the PR, I really appreciate it, but @lucacasonato and I took a deeper look at it and we found out we needed to do a lot more changes. We've opened a new PR at #9041. Closing without merge.

@bartlomieju bartlomieju closed this Jan 7, 2021
@yos1p
Copy link
Contributor Author

yos1p commented Jan 7, 2021

No worries. Looking forward for the new PR!

@yos1p yos1p deleted the lite-binary branch January 7, 2021 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants